Fix Windows background test-run signal coverage - #377

Sorry @unbraind, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Code Review

This pull request hardens the background test run lifecycle tests on Windows by replacing child process self-termination (process.kill) with process-level event emission (process.emit) from the parent test process once progress is detected in the stderr log. It also updates an MCP dynamic package integration test to use a dry-run option to avoid network requests. The reviewer identified a potential race condition in one of the modified tests due to a hardcoded 50ms timeout for emitting SIGTERM and suggested polling the stderr log for progress instead to ensure test stability on slow CI environments.

Code Review

This pull request addresses Windows Vitest worker self-termination issues in background-run signal tests by refactoring signal handling to use parent process event emission instead of child process kills, and updates the MCP dynamic package upgrade test to run as a dry-run. The review feedback highlights potential flakiness in the test-runs-command tests: first, using a hardcoded 50ms timeout to emit SIGTERM is prone to race conditions and should be replaced with a polling mechanism; second, the 500ms polling limit in the progress-ready signal helper should be increased to 10 seconds to ensure stability on slower CI environments.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/unit/commands/test-runs-command.spec.ts`:
- Around line 795-812: The `signalWhenProgressReady` helper in
`runBackgroundTestRunWorker` is racing on slow Windows CI because it gives up
too quickly before `stderr` contains `id=pm-stop`. Increase the polling budget
in that helper (either by more attempts or a longer fixed timeout) so the marker
is observed before emitting `SIGTERM` and `SIGINT`, keeping the `stopped`
assertions deterministic.

Code Review

This pull request updates issue tracking logs and hardens tests to prevent Vitest worker self-termination on Windows. Specifically, in tests/unit/commands/test-runs-command.spec.ts, the background test run lifecycle tests are modified to emit SIGTERM and SIGINT signals directly from the test process via process.emit once progress is detected, rather than relying on child processes killing the parent process. Additionally, the MCP dynamic package action integration test is updated to run as a dry run. The reviewer suggested using Promise.prototype.finally() in one of the test blocks for consistency with other similar implementations in the file.

Code Review

This pull request updates tracking files, configures the MCP dynamic package upgrade integration test to use a dry-run option, and refactors background test run lifecycle tests to emit process signals locally instead of calling process.kill on the parent process. The review feedback highlights that emitting global SIGTERM and SIGINT events on the process object can still trigger Vitest's own signal handlers, potentially causing unexpected worker exits. It suggests mocking process.on and process.off to capture and invoke the registered signal handlers directly, thereby isolating the signal simulation.

Code Review

This pull request hardens the background test run lifecycle tests on Windows by replacing real OS signal termination with mocked process event listeners, and updates the MCP dynamic package upgrade test to run as a dry-run. The review feedback correctly identifies a potential test hang in the new polling logic if the background worker fails or exits early, and suggests introducing a flag to terminate the polling loop immediately.

Code Review

This pull request resolves Windows Vitest worker self-termination issues in background-run signal tests by replacing real process signals with mocked process.on and process.off handlers to simulate SIGTERM and SIGINT events. It also updates an integration test to run the MCP upgrade action as a dry-run to keep it network-free. The reviewer points out that mocking global singletons like process.on and process.off without delegating unhandled events to their original implementations can lead to flaky tests, and provides code suggestions to delegate non-targeted events to their original implementations.

Code Review

This pull request refactors the background test run lifecycle tests in tests/unit/commands/test-runs-command.spec.ts to prevent Windows Vitest worker self-termination. Instead of having child processes send signals to the parent process via process.kill, the tests now capture and manually invoke registered SIGTERM and SIGINT handlers using a new captureProcessSignalHandlers helper. Additionally, the MCP dynamic package upgrade test is updated to use a dry-run option to keep it network-free. The review feedback correctly identifies that the immediately-invoked function expressions (IIFEs) used for signal polling should have .catch(() => {}) appended directly to them to prevent potential unhandled promise rejections.

Code Review

This pull request hardens the background test run lifecycle tests on Windows by capturing and manually triggering SIGTERM and SIGINT handlers instead of using process.kill, which was causing Vitest worker self-termination. It also updates MCP dynamic package actions to use a dry-run option. The reviewer suggests extending the signal-capturing mock to cover process.addListener and process.removeListener aliases for completeness and robustness.

Code Review

This pull request hardens the background test run lifecycle tests in tests/unit/commands/test-runs-command.spec.ts to prevent Vitest worker self-termination on Windows. Instead of having child processes send signals to the parent process, the tests now capture and manually invoke the registered SIGTERM and SIGINT handlers once progress is observed in the stderr log. Additionally, the MCP dynamic package upgrade test in tests/integration/mcp-dynamic-package-actions.spec.ts has been updated to use a dry-run option to keep it network-free and stable. The issue tracking files have also been updated to reflect these changes. As there are no review comments, I have no feedback to provide.

Code Review

This pull request refactors background test-run signal tests in tests/unit/commands/test-runs-command.spec.ts to capture and trigger signal handlers manually, preventing Vitest worker self-termination on Windows. It also updates tests/integration/mcp-dynamic-package-actions.spec.ts to use a dry-run option for the upgrade action. Feedback on the changes suggests simplifying the offSpy implementation in the signal capture helper by consolidating redundant fallback return statements.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/unit/commands/test-runs-command.spec.ts`:
- Around line 855-877: The signal-stop and force-kill tests both reimplement the
same polling-and-signal dispatch logic, so extract that block into a shared
helper to prevent divergence. Move the `signalWhenProgressReady` behavior into a
reusable function (for example near the affected test cases) parameterized by
the stderr marker and the signal handler groups, and reuse it from both the
existing test around the `sigtermHandlers`/`sigintHandlers` flow and the
force-kill test so the `progressObserved`/`workerFinished` guard stays
consistent in one place.

---

Outside diff comments:
In @.agents/pm/issues/pm-2kkl.toon:
- Around line 17-51: The issue synopsis metadata is stale and still points to
GH-348 / Node 20 plus the old 4-spec regression scope, while the comment log now
reflects GH-376 / Windows Node 24 and the docs-gate unblock. Update the
canonical issue fields in this file’s top-level title, description, and body so
they match the current incident and verification state. Keep the synopsis
aligned with the latest notes and regression coverage referenced in the issue
log so tooling and triage read the correct status.

Code Review

This pull request hardens the background test-run signal tests to prevent Vitest worker self-termination on Windows. It introduces captureProcessSignalHandlers and dispatchSignalsWhenProgressReady helpers in tests/unit/commands/test-runs-command.spec.ts to mock and simulate SIGTERM and SIGINT signals in-process instead of sending real signals via process.kill. Additionally, it updates tests/integration/mcp-dynamic-package-actions.spec.ts to run the MCP upgrade action as a dry run to keep it network-free. The reviewer's feedback suggests clearing the interval timer gracefully in the mock script when stopped to avoid relying on short force-kill delays, which can cause flakiness on slower CI environments.

Code Review

This pull request addresses the Windows nightly Vitest worker fork unexpected exit issue (GH-348/GH-376). It hardens the background test-run signal tests in test-runs-command.spec.ts by capturing and invoking SIGTERM/SIGINT handlers programmatically rather than sending real signals that could terminate the parent Vitest worker. Additionally, it updates the MCP dynamic package upgrade integration test to run as a dry-run, keeping it network-free and stable. I have no review comments or feedback to provide as the changes are well-implemented and directly address the target issues.